Add table extension to mdConverter goldmark renderer#498
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds Markdown table support to the rich-text Markdown→HTML converter.
Changes:
- Enable Goldmark’s
extension.Tablealongsideextension.Strikethrough
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
Thanks for this. Just wanted to drop a note that the eventual goal is to have markdown conversion done server-side (not in the CLI). But that's going to be a few more weeks at least, so it may make sense to add this client-side for now. I'll defer to @jeremy! |
|
No worries Mike, thanks for the explanation, I was wondering why the conversion was done locally instead of server-side, makes sense now. Appreciate your help, have a great day. |
There was a problem hiding this comment.
Thanks for this — registering the table extension is exactly the fix for #405. One correction so column alignment survives BC3's sanitizer, plus some surrounding work that doesn't fit an inline suggestion.
The alignment gotcha (inline suggestion below): plain extension.Table emits alignment as style="text-align:…". BC3's ContentPipeline sanitizer strips inline style (only color/background-color survive) but whitelists the align attribute — verified in bc3 app/helpers/content_pipeline/sanitization.rb. Configuring the extension with TableCellAlignAttribute emits align="left"/align="right" instead, so GFM column alignment is preserved through sanitization. Bare <table> is still correct — BC3's WrapTablesFilter supplies the <figure class="lexxy-content__table-wrapper"> wrapper.
The rest (can't be inline): because the CLI can now author tables, a few adjacent paths need to change so tables aren't silently destroyed — IsMarkdown needs AST-based table detection (a table-only chat message is otherwise sent as raw pipes), IsHTML needs to recognize <table> (a plain table response otherwise leaks raw markup on display), and the TUI in-place editors need to fail closed on table-bearing content (HTMLToMarkdown has no table handling, so opening + resubmitting would flatten the table — round-tripping waits on BC3 #11986). I've pushed all of that, with tests and a skill-doc boundary note, as a reference branch you're welcome to fold in: fix/markdown-tables (commit 601b272). bin/ci is fully green on it.
Register goldmark's table extension in mdConverter so Markdown tables convert to <table> HTML instead of leaking as raw pipe rows. Use the align-attribute cell method rather than the default inline style, since BC3's sanitizer whitelists the align attribute but strips style, so column alignment survives sanitization. BC3's WrapTablesFilter supplies the figure wrapper, so a bare <table> is correct. Because the CLI can now author tables, guard against destroying them: - IsMarkdown gains AST-based table detection so a table-only chat message is sent as HTML rather than verbatim pipes. - IsHTML recognizes <table> so a plain table response is routed through the display converter instead of leaking raw markup to the reader. - The TUI in-place editors (message body, comment, to-do description) fail closed on table-bearing content: HTMLToMarkdown has no table handling, so they refuse to open rather than flatten the table on resubmit. Round-trip editing waits on server-side Markdown (BC3 #11986). Document the table boundary in the Basecamp skill.
github-actions
Bot
added
tui
|
@savtrip I pushed the reconciled work onto this branch, rebased onto current
|
The docstring claimed the detector matched self-closing table tags, but the character class [\s>] excluded '/'. Add '/' so <table/> is caught, keeping the boundary that prevents <tablefoo> from matching.
IsMarkdown runs hasMarkdownTable on every input that matches none of the regex patterns — typically plain text on TUI submit/editor return. A GFM table can't exist without both a cell-delimiting '|' and a delimiter row on its own line, so skip the full parse when either is absent. This mirrors goldmark's own behavior (it treats a CR-only table with no '\n' as not a table), so no real table is missed.
|
Hi @jeremy excellent, thanks for adding the necessary robustness around the PR. The code looks good to me, I can't make comment on bc3 required changes (I run bc5) but it looks good to me. If you were wanting further testing I am more than happy to contribute. Thanks for getting this change, I've been wrangling with my agent for the last month to add tables in HTML, they love exporting to markdown lol -- let me know if you need anything from me. |
|
Thanks @savtrip! |
What
Hi guys,
This adds the goldmark table extension for rendering markdown tables to HTML that lexxy now supports in bc5.
Here are the docs for the table extension that I added: https://pkg.go.dev/github.com/yuin/goldmark#readme-built-in-extensions
Why
My agent was adding tables to descriptions of cards and todos (any rich text area) but I noticed the table was not rendering but instead showed the raw markdown.
Therefore, I looked into the code and discovered there was no table extension for goldmark (refer to the docs I linked above to see the table extension). Also, I noticed from the comment it says:
// mdConverter is the goldmark Markdown-to-HTML converter configured for Trix compatibility.Therefore, I assume the team has not optimised for lexxy yet given it says Trix.
If my agent renders everything in HTML then the table works fine. But sometimes it chooses to do it in markdown and therefore I end up having the raw markdown content in my rich text (which obviously looks terrible lol).
If for whatever reason you guys aren't happy with the change then writing something in the SKILL file to say "Note: tables will not render if provided in markdown because the CLI does not convert them to HTML" would at least signal to the agent that it's not possible.
Testing
make checkpassesI did not add any test cases in. I performed a cli test of adding a card and it worked successful. I did not want to add tests because I wanted to check if this was a change you guys were happy with.
EDIT: Screenshots
I added the screenshots below to show the problem in basecamp.
HTML
Markdown
Summary by cubic
Enable Markdown table rendering by adding the
goldmarktable extension; tables now render as HTML withalignattributes so GFM alignment survives BC3 sanitization.Also improves detection and safety: table-only Markdown is detected via the AST with a fast pre-check to skip parsing when no
, TUI editors refuse table-bearing content to prevent data loss, and HTML detection now also matches self-closing|or newline,IsHTMLrecognizesWritten for commit 5b5903a. Summary will update on new commits.